Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feature: add prebind for scheduler #3621

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feature: add prebind for scheduler #3621

wants to merge 1 commit into from

Conversation

googs1025
Copy link
Member

@googs1025 googs1025 commented Jul 25, 2024 •
edited
Loading

/kind feature

  • add prebindFn for scheduler

fix: #3618

part of issue: #3618

@volcano-sh-bot volcano-sh-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/feature Categorizes issue or PR as related to a new feature. labels Jul 25, 2024
@volcano-sh-bot volcano-sh-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 25, 2024
@googs1025 googs1025 force-pushed the prebind_feature branch 4 times, most recently from dfa4ced to 31fbc9c Compare July 25, 2024 08:29
@Monokaix
Copy link
Member

It's not just simply add a prebind，we should also call unallocate to do the reverse in framework, you can refer to the kube-scheduler logic: )

@googs1025
Copy link
Member Author

It's not just simply add a prebind，we should also call unallocate to do the reverse in framework, you can refer to the kube-scheduler logic: )

As far as I understand, can we split it into smaller PRs to complete different features? Because adding the prebind method does not seem to be related to the unallocate method.

@googs1025
Copy link
Member Author

It's not just simply add a prebind，we should also call unallocate to do the reverse in framework, you can refer to the kube-scheduler logic: )

I am not sure if I need to implement a method similar to unreserve, because there is no process similar to reserve in the volcano scheduler process. Maybe pipeline is a process similar to reserve?

@googs1025 googs1025 force-pushed the prebind_feature branch 2 times, most recently from 5e4c5db to 722c283 Compare July 30, 2024 09:30
googs1025
googs1025 commented Jul 30, 2024
View reviewed changes
pkg/scheduler/framework/session.go Outdated
@@ -437,6 +438,41 @@ func (ssn *Session) Allocate(task *api.TaskInfo, nodeInfo *api.NodeInfo) (err er
return nil
}

// Unallocate is called when the prebind function fails
func (ssn *Session) Unallocate(task *api.TaskInfo) error {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic of the Unallocate method is to modify the cache in ssn

  1. Change the task status to pending
  2. Delete this task (Pod) in the node
  3. send Deallocate event (I'm not sure if this step is necessary)
  4. set task.Name = = ""

@googs1025
Copy link
Member Author

It's not just simply add a prebind，we should also call unallocate to do the reverse in framework, you can refer to the kube-scheduler logic: )

added

@googs1025 googs1025 changed the title WIP: feature: add prebind for scheduler feature: add prebind for scheduler Jul 30, 2024
@volcano-sh-bot volcano-sh-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 30, 2024
@googs1025
Copy link
Member Author

/assign @Monokaix

lowang-bh
lowang-bh reviewed Jul 30, 2024
View reviewed changes
pkg/scheduler/framework/session.go Outdated
@@ -437,6 +438,41 @@ func (ssn *Session) Allocate(task *api.TaskInfo, nodeInfo *api.NodeInfo) (err er
return nil
}

// Unallocate is called when the prebind function fails
func (ssn *Session) Unallocate(task *api.TaskInfo) error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It exists in statement.

lowang-bh
lowang-bh reviewed Jul 30, 2024
View reviewed changes
pkg/scheduler/actions/backfill/backfill.go
@@ -91,6 +91,17 @@ func (backfill *Action) Execute(ssn *framework.Session) {
}
}

// before allocating to a node, run prebind fn
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What will preBind be used for? And why don't put it in ssn.Allocate?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue detail: #3618

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs @Monokaix to help explain.
I think it is a method to expand to adapt to k8s plugins.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because k8s is always changing, I'm not sure if this incremental adaptation is a good design approach.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As described in issue，without preBind plugin's logic is scattered in framework，for volumeBinidng plugin, both framewok and plugin hold cache and lead to inconsistent cache and memory leak, it's also unacceptable.
If you guys have better thoughts, any suggestions are welcome: )

@googs1025
feature: add prebind for scheduler
cab9e4f 
Signed-off-by: googs1025 <googs1025@gmail.com>
@volcano-sh-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign monokaix
You can assign the PR to them by writing /assign @monokaix in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Monokaix Monokaix Monokaix left review comments

@lowang-bh lowang-bh lowang-bh left review comments

@merryzhou merryzhou Awaiting requested review from merryzhou

At least 1 approving review is required to merge this pull request.

Assignees

@Monokaix Monokaix

Labels
kind/feature Categorizes issue or PR as related to a new feature. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Introduce a new interface AddPreBindFn or AddBindFn in session
4 participants
@googs1025 @Monokaix @volcano-sh-bot @lowang-bh